Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move C functions to pybind with GIL release #6204

Merged
merged 4 commits into from
Oct 2, 2023

Conversation

andreas-el
Copy link
Contributor

@andreas-el andreas-el commented Sep 28, 2023

Issue
Resolves #6199

Releasing the GIL with pybind

        // release the GIL
         py::gil_scoped_release release;

This seem to work fine, mostly due to this being a relatively simple function, and the data inside it is guarded by the job_node mutex.

Regarding confirmed_running:

We determined that the way the refresh function was refactored, it was reading/checking for the same file over-and-over again needlessly. This way we limit the checking from when the state is JOB_QUEUE_RUNNING and only until the existence of the file is proven.

 if (current_status & JOB_QUEUE_RUNNING && !node->confirmed_running) {
             node->confirmed_running =
                 !node->status_file || fs::exists(node->status_file);

Re-add submit+kill functions that was reverted earlier

A month ago we reverted some functions that was ported from cwrap to pybind, where the experience was a slowdown/lockdown of the whole application.
Using gil_scoped_release we can re-add these functions again, with the same premises as other code in this PR.

Pre review checklist

  • Read through the code changes carefully after finishing work
  • Make sure tests pass locally (after every commit!)
  • Prepare changes in small commits for more convenient review (optional)
  • PR title captures the intent of the changes, and is fitting for release notes.
  • Updated documentation
  • Ensured that unit tests are added for all new behavior (See
    Ground Rules),
    and changes to existing code have good test coverage.

Pre merge checklist

  • Added appropriate release note label
  • Commit history is consistent and clean, in line with the contribution guidelines.

@andreas-el andreas-el changed the title Pybind gil release Refactor and move job node refresh to pybind Sep 28, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 28, 2023

Codecov Report

Merging #6204 (b373edd) into main (b577c65) will decrease coverage by 0.04%.
Report is 3 commits behind head on main.
The diff coverage is 9.87%.

@@            Coverage Diff             @@
##             main    #6204      +/-   ##
==========================================
- Coverage   82.65%   82.61%   -0.04%     
==========================================
  Files         347      347              
  Lines       21247    21251       +4     
  Branches      834      849      +15     
==========================================
- Hits        17562    17557       -5     
- Misses       3387     3396       +9     
  Partials      298      298              
Files Coverage Δ
src/ert/job_queue/job_queue_node.py 91.83% <100.00%> (-0.17%) ⬇️
src/clib/lib/job_queue/job_node.cpp 14.63% <0.00%> (-0.47%) ⬇️

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Release GIL in submit and kill functions
@andreas-el andreas-el marked this pull request as ready for review September 29, 2023 10:22
@andreas-el andreas-el changed the title Refactor and move job node refresh to pybind Move C functions to pybind with GIL release Sep 29, 2023
@andreas-el andreas-el added the release-notes:maintenance Automatically categorise as maintenance change in release notes label Sep 29, 2023
Copy link
Contributor

@eivindjahren eivindjahren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@andreas-el andreas-el merged commit 510f483 into equinor:main Oct 2, 2023
@andreas-el andreas-el deleted the pybind_gil_release branch October 2, 2023 06:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes:maintenance Automatically categorise as maintenance change in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate releasing the Global Interpreter Lock with pybind
3 participants